Allow the leader election resource lock type to be configurable by mprahl · Pull Request #1448 · openshift/library-go

mprahl · 2023-01-06T15:11:49Z

The controllercmd package always utilized a configmapsleases resource lock for leader election, but some client may entirely want to move to leases. This commit keeps the default to configmapsleases but allows it to be configurable to whatever the
k8s.io/client-go/tools/leaderelection/resourcelock package allows.

Related issues:
#1178
https://issues.redhat.com/browse/ACM-2653

mprahl · 2023-01-06T15:13:42Z

/cc deads2k soltysh sttts

mprahl · 2023-01-13T13:53:41Z

Could I please get a review on this PR?

pkg/config/leaderelection/leaderelection.go

pkg/controller/controllercmd/builder.go

mprahl · 2023-01-26T13:28:16Z

@dgrisonnet could you please provide your thoughts on my last comment?

pkg/controller/controllercmd/builder.go

mprahl · 2023-02-03T20:19:24Z

Okay, the PR is updated.

mprahl · 2023-02-06T17:34:16Z

@dgrisonnet is this good to merge then?

dgrisonnet · 2023-02-06T15:25:30Z

pkg/config/leaderelection/leaderelection.go

 // See https://github.com/kubernetes/kubernetes/issues/107454 for
 // details on how to migrate to "leases" leader election.
 // Don't forget the callbacks!
-// TODO: In the next version we should switch to using "leases"


@tkashem did you intend to switch the default to leases instead of configmapleases? If so we might want to keep this TODO until the default resource is updated.

@dgrisonnet and @tkashem I moved the TODO to where the logic for the default resource lock is made. Is this okay with you?

Yes, actually, that was the goal

we want the following:

A: a brand new operator being written should us lease (preferably by default)

B: existing operators who are currently using configmapsleases should use lease (preferably by default)

C: existing operators who have not yet upgraded libray-go for a while (still using configmaps) should start using configmapsleases as soon as they upgrade to the latest library-go (we can't have these operators use lease by default, it will break their upgrade flow)

I am concerned about C - if we don't have any existing operators in C category today then we can safely rename ToLeaderElectionWithConfigmapLease to ToLeaderElectionWithLease and have lease be the default option.
So I would like to know if C is empty today - it can be possible today to count C, maybe by having a test in CI?

or is there a way we can break (static, compile time) these operators in C when they upgrade library-go so they can opt-in? I would like us to be able to avoid these changes if possible, especially if C is empty.

Leases are not 100% reliable today. It's been a while since the switch I think, I'd simply switch.

@mprahl can you revise the pr to switch to lease? (disregard C, that means no need to make anything configurable)

@tkashem in ACM we would like to still have the option for configmapsleases if that's okay. I'd make lease the default though.

@mprahl yeah, sure, go ahead. (just curious, you have an operator that is currently using configmaps?)

The controllercmd package always utilized a configmapsleases resource lock for leader election, but some client may entirely want to move to leases. This commit keeps the default to configmapsleases but allows it to be configurable of current best practices. Related issues: #1178 https://issues.redhat.com/browse/ACM-2653 Signed-off-by: mprahl <mprahl@users.noreply.github.com>

openshift-ci · 2023-02-13T13:53:10Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: mprahl
Once this PR has been reviewed and has the lgtm label, please assign sttts for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

mprahl · 2023-02-13T13:53:50Z

/retest

tkashem · 2023-02-16T00:32:24Z

pkg/controller/controllercmd/builder.go


+// WithLeaderElectionResourceLock sets the resource lock. If not set, controllercmd.ConfigMapsLeasesResourceLock will
+// be used for backwards compatibility.
+func (b *ControllerBuilder) WithLeaderElectionResourceLock(resourceLock leaderElectionResourceLock) *ControllerBuilder {


instead of using the lock as a parameter, can we have two methods?

WithLeaderElectionLeaseResourceLock() *ControllerBuilder WithLeaderElectionConfigmapLeaseResourceLock() *ControllerBuilder

this way we don't have to have validation around resource lock names and in future we can simply remove WithLeaderElectionConfigmapLeaseResourceLock

tkashem · 2023-02-16T00:35:17Z

pkg/config/leaderelection/leaderelection.go

+// ToLeaderElectionWithLease returns a "leases" based leader
+// election config that you just need to fill in the Callback for.
+// Don't forget the callbacks!
+func ToLeaderElectionWithLease(clientConfig *rest.Config, config configv1.LeaderElection, component, identity string) (leaderelection.LeaderElectionConfig, error) {


we can remove ToLeaderElectionWithLease and restore toLeaderElection to ToLeaderElection

tkashem · 2023-02-16T00:36:51Z

pkg/controller/controllercmd/builder.go

+		return fmt.Errorf("%w: %s", ErrInvalidResourceLock, b.leaderElectionResourceLock)
+	}

-	leaderElection, err := leaderelectionconverter.ToLeaderElectionWithConfigmapLease(leaderConfig, *b.leaderElection, b.componentName, b.instanceIdentity)


use proposed eaderelectionconverter.ToLeaderElection(leaderConfig, *b.leaderElection, b.componentName, b.instanceIdentity, b.leaderElectionResourceLock

we can get rid of the switch here

tkashem · 2023-02-16T00:41:44Z

pkg/controller/controllercmd/cmd.go


+	// LeaderElectionResourceLock sets the resource lock. If not set, controllercmd.ConfigMapsLeasesResourceLock will
+	// be used for backwards compatibility.
+	LeaderElectionResourceLock leaderElectionResourceLock


we need to know whether it's lease or configmapsleases, so can we make it a boolean

// DEPRECATED: at some point everything should uses lease by default OverrideConfigmapsLeasesResourceLock bool

This will allow new operators to opt in for lease

tkashem · 2023-02-16T00:44:42Z

pkg/controller/controllercmd/cmd.go

 		WithKubeConfigFile(c.basicFlags.KubeConfigFile, nil).
 		WithComponentNamespace(c.basicFlags.Namespace).
 		WithLeaderElection(config.LeaderElection, c.basicFlags.Namespace, c.componentName+"-lock").
+		WithLeaderElectionResourceLock(c.LeaderElectionResourceLock).


replace it with

builder = builder. WithLeaderElectionConfigmapLeaseResourceLock() if c.OverrideConfigmapsLeasesResourceLock { builder = builder. WithLeaderElectionLeaseResourceLock() }

mprahl · 2023-04-11T15:02:38Z

Closing in favor of:
caaf57d

mprahl mentioned this pull request Jan 6, 2023

Migrate LeaderElection to Lease API #1178

Closed

openshift-ci bot requested review from deads2k, soltysh and sttts January 6, 2023 15:13

dgrisonnet reviewed Jan 13, 2023

View reviewed changes

pkg/config/leaderelection/leaderelection.go Outdated Show resolved Hide resolved

dgrisonnet reviewed Jan 13, 2023

View reviewed changes

pkg/controller/controllercmd/builder.go Outdated Show resolved Hide resolved

mprahl requested review from dgrisonnet and removed request for deads2k, soltysh and sttts January 18, 2023 16:08

dgrisonnet reviewed Feb 3, 2023

View reviewed changes

pkg/controller/controllercmd/builder.go Show resolved Hide resolved

mprahl requested review from dgrisonnet and tkashem and removed request for dgrisonnet February 3, 2023 18:58

mprahl requested review from dgrisonnet and removed request for tkashem February 6, 2023 13:16

dgrisonnet reviewed Feb 6, 2023

View reviewed changes

tkashem reviewed Feb 16, 2023

View reviewed changes

mprahl closed this Apr 11, 2023

Comments

Conversation

mprahl commented Jan 6, 2023

Uh oh!

mprahl commented Jan 6, 2023

Uh oh!

mprahl commented Jan 13, 2023

Uh oh!

Uh oh!

Uh oh!

mprahl commented Jan 26, 2023

Uh oh!

Uh oh!

mprahl commented Feb 3, 2023

Uh oh!

mprahl commented Feb 6, 2023

Uh oh!

dgrisonnet Feb 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

openshift-ci bot commented Feb 13, 2023

Uh oh!

mprahl commented Feb 13, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tkashem Feb 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mprahl commented Apr 11, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

dgrisonnet Feb 6, 2023 •

edited

Loading

tkashem Feb 16, 2023 •

edited

Loading